Skip to content

chore: add scripts/gen-semconv-ts.js to help generate semconv.ts files for unstable semconv consts #2669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
trentm merged 6 commits into from
Feb 20, 2025
Merged

chore: add scripts/gen-semconv-ts.js to help generate semconv.ts files for unstable semconv consts #2669

trentm merged 6 commits into from
Feb 20, 2025

Conversation

@trentm
Copy link
Contributor

@trentm trentm commented Jan 18, 2025

Refs: https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv

Now that we've established how we want packages to use unstable semconv definitions, we are starting to get PRs adding a local copy of these: #2664, #2668

It is helpful to script this to: make it faster, encourage a common pattern for these local copies, avoid cut 'n paste errors.

An example run of this (for #2668) looks like this:

% ./scripts/gen-semconv-ts.js detectors/node/opentelemetry-resource-detector-aws
Found import of 31 unstable semconv definitions.
Generated "detectors/node/opentelemetry-resource-detector-aws/src/semconv.ts".
Running 'npx eslint --fix src/semconv.ts' to fix formatting.

It will also warn/error on attempts to put stable semconv exports in there. That looks like this:

% ../../../scripts/gen-semconv-ts.js
gen-semconv-ts warning: ./src/detectors/AwsBeanstalkDetectorSync.ts: 'ATTR_SERVICE_NAME' export is available on the stable "@opentelemetry/semantic-conventions" entry-point. This definition will not be included in the generated semconv.ts. Instead use:
    import { ATTR_SERVICE_NAME } from '@opentelemetry/semantic-conventions';
gen-semconv-ts warning: ./src/detectors/AwsBeanstalkDetectorSync.ts: 'ATTR_SERVICE_VERSION' export is available on the stable "@opentelemetry/semantic-conventions" entry-point. This definition will not be included in the generated semconv.ts. Instead use:
    import { ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions';
Found import of 31 unstable semconv definitions.
Generated "src/semconv.ts".

@trentm trentm self-assigned this Jan 18, 2025
@trentm trentm requested a review from a team as a code owner January 18, 2025 00:54
trentm added a commit to garysassano/opentelemetry-js-contrib that referenced this pull request Jan 18, 2025
@trentm
use gen-semconv-ts.js script from open-telemetry#2669 to generate sem…
0ac7ca1 
…conv.ts file
@trentm trentm mentioned this pull request Jan 18, 2025
Merged
@codecov
Copy link

codecov bot commented Jan 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.41%. Comparing base (d2a9a20) to head (3484f6e).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2669      +/-   ##
==========================================
- Coverage   92.46%   92.41%   -0.05%     
==========================================
  Files         171      171              
  Lines        8150     8150              
  Branches     1653     1653              
==========================================
- Hits         7536     7532       -4     
- Misses        614      618       +4

see 1 file with indirect coverage changes

@trentm trentm mentioned this pull request Feb 5, 2025
Closed
pichlermarc
pichlermarc approved these changes Feb 11, 2025
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, that's pretty cool 🙂

I've tried it out and it seems that it also includes the @experimental note about how breaking changes in the semconv package would affect it. Not sure if it would make sense to replace those with an empty string or something like "generated from experimental semconv". 🤔

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

I've tried it out and it seems that it also includes the @experimental note about how breaking changes in the semconv package would affect it.

Looking at an example (for experimental/packages/web-common):

/**
 * A unique id to identify a session.
 *
 * @example "00112233-4455-6677-8899-aabbccddeeff"
 *
 * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`.
 */
export const ATTR_SESSION_ID = 'session.id';

It is still accurate, no? I.e. that it could break in @opentelemetry/semantic-conventions.
I guess a maintainer could be confused, given that the copied var is no longer in that package.

The generated top-comment:

/*
 * This file contains a copy of unstable semantic convention definitions
 * used by this package.
 * @see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv
 */

is meant to help give context.

I think it could be helpful to the maintainer/reader to know that the const is still experimental in semconv.

Thoughts?

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

Not sure if it would make sense to replace those with an empty string or something like "generated from experimental semconv".

Or even include the semconv version in that sentence. 🤔

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

@pichlermarc @dyladan Separate question: the semconv constants have as const;. Currently this script does not add that (mainly because it is getting the content from the local non-TS build/esnext/experimental_*.js files). What does the as const; accomplish?

@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

What does the as const; accomplish?

Answering myself: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#const-assertions

@trentm
if no unstable semconv usages found, then don't write a src/semconv.t…
629f255 
…s file
@trentm
update the script to use *typescript* sources to copy from, rather th…
c058095 
…an build/esnext/... sources in a local npm install.

This does a shallow git clone if necessary.
@trentm
Copy link
Contributor Author

trentm commented Feb 11, 2025

Commit c058095 adds the as const suffix if it is in the TypeScript sources. It does this by using the TS sources now, instead of build/esnext/*.js files in a local npm install.

@trentm trentm mentioned this pull request Feb 12, 2025
Merged
@pichlermarc
Copy link
Member

pichlermarc commented Feb 20, 2025

The generated top-comment:

/*
 * This file contains a copy of unstable semantic convention definitions
 * used by this package.
 * @see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv
 */

is meant to help give context.

I think it could be helpful to the maintainer/reader to know that the const is still experimental in semconv.

Thoughts?

Ah, sorry I did not see that one. All good 👍

@trentm trentm enabled auto-merge (squash) February 20, 2025 22:32
@trentm trentm merged commit 3fc62be into open-telemetry:main Feb 20, 2025
4 checks passed
@trentm trentm deleted the tm-gen-semconv-ts branch February 20, 2025 22:40
deejay1 pushed a commit to deejay1/opentelemetry-js-contrib that referenced this pull request Apr 14, 2025
@trentm @deejay1
chore: add scripts/gen-semconv-ts.js to help generate src/semconv.ts …
8b6c510 
…files for unstable semconv consts (open-telemetry#2669)

Refs: https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pichlermarc pichlermarc pichlermarc approved these changes

Assignees

@trentm trentm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@trentm @pichlermarc